Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve spacing around text nodes and braces #97

Merged
merged 15 commits into from
Mar 7, 2019
Merged

Conversation

chinedufn
Copy link
Owner

@chinedufn chinedufn commented Mar 6, 2019

Fixes #83

Also refactors a good chunk of the html-macro to make it easier to read and a good bit more DRY

@chinedufn chinedufn requested a review from dbrgn March 6, 2019 14:20
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really review the logic, but looks good overall!

crates/html-macro/src/parser/close_tag.rs Outdated Show resolved Hide resolved
crates/html-macro/src/parser/mod.rs Outdated Show resolved Hide resolved
crates/html-macro/src/parser/mod.rs Outdated Show resolved Hide resolved
/// Parse a sequence of tokens until we run into a closing tag
/// html! { <div> Hello World </div> }
/// or a brace
/// html! { <div> Hello World { Braced } </div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing curly brace missing?

text += &tt.to_string();
// Insert necessary space in between the tokens in this text node that we're building.
// In the real browser DOM there's no difference between one space and many,
// so we just insert one.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not necessarily true! If you use white-space: pre or white-space: pre-wrap CSS rules then white-space will not be collapsed.

https://developer.mozilla.org/en-US/docs/Web/CSS/white-space

Not sure if that could be a problem or not.

Copy link
Owner Author

@chinedufn chinedufn Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh thanks for catching this!

Luckily the fix here is straightforwards enough.

We'll just look at the spans for the text tokens and see how far apart they are - then insert that much space in between then.

And if their spans are on different lines we'll just insert the right amount of new line characters followed by spacing whatever amount of spacing we need.


Also in text.rs we'll need to insert the right amount of space and newlines before/after the entire string of text.. vs. right now we just insert a single space

if should_insert_space_after_text {
text += " ";
}


Thanks for catching this - I'll add some more test cases and get this fixed. Then we should be in awesome shape here!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look and think there's a better solution.. Going to merge this and open a separate issue.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#98

dbrgn and others added 5 commits March 6, 2019 14:44
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
@chinedufn chinedufn merged commit 8ac5e30 into master Mar 7, 2019
@chinedufn chinedufn deleted the text-space branch March 7, 2019 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants